Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for Croatian TimeSpan localization bug #598

Merged
merged 1 commit into from
Dec 2, 2016

Conversation

hangy
Copy link
Contributor

@hangy hangy commented Nov 30, 2016

This fixes #597 by using the same value for both resource keys. As far as I found out "dana" is the general plural for days, and the postfix is not required. However, the method that gets the resource key does not know that it is not formatting numbers but timespans at this point, so I think this workaround is the best available quick fix for the bug.

@hangy
Copy link
Contributor Author

hangy commented Nov 30, 2016

Apologies in advance, if the first CI tests fail and I need to re-adjust something. I'm having a bit of a problem getting VS to run the 22k unit tests, and the build.cmd doens't seem to work well right now, either.

@clairernovotny
Copy link
Member

I'll see what I can do about fixing build.cmd, but in the mean time, three tests are failing:


11/30/2016 11:10:14 PM ~000031ms Variable: frame = 'Humanizer.Tests.PublicApiApprovalTest.approve_public_api()'

    Days(days: 40, expected: "40 dana") [FAIL]

      Assert.Equal() Failure

                � (pos 0)

      Expected: 40 dana

      Actual:   5 tjedana

                � (pos 0)

      Stack Trace:

        src\Humanizer.Tests.Shared\Localisation\hr\TimeSpanHumanizeTests.cs(17,0): at Humanizer.Tests.Localisation.hr.TimeSpanHumanizeTests.Days(Int32 days, String expected)

    Days(days: 41, expected: "42 dana") [FAIL]

      Assert.Equal() Failure

                � (pos 0)

      Expected: 42 dana

      Actual:   5 tjedana

                � (pos 0)

      Stack Trace:

        src\Humanizer.Tests.Shared\Localisation\hr\TimeSpanHumanizeTests.cs(17,0): at Humanizer.Tests.Localisation.hr.TimeSpanHumanizeTests.Days(Int32 days, String expected)

    Days(days: 42, expected: "42 dana") [FAIL]

      Assert.Equal() Failure

                � (pos 0)

      Expected: 42 dana

      Actual:   6 tjedana

                � (pos 0)

      Stack Trace:

        src\Humanizer.Tests.Shared\Localisation\hr\TimeSpanHumanizeTests.cs(17,0): at Humanizer.Tests.Localisation.hr.TimeSpanHumanizeTests.Days(Int32 days, String expected)

  Finished:    Humanizer.Tests

=== TEST EXECUTION SUMMARY ===

   Humanizer.Tests  Total: 7261, Errors: 0, Failed: 3, Skipped: 0, Time: 2.931s

Command exited with code 3

commit f9423de
Author: hangy <hangy@hangy.de>
Date:   Thu Dec 1 20:48:01 2016 +0100

    Update TimeSpanHumanizeTests.cs

commit 2e5dfe5
Author: hangy <hangy@hangy.de>
Date:   Thu Dec 1 20:37:50 2016 +0100

    Apparently I'm stupid and 7 days actually get humanized as 1 week.

commit 152a119
Author: hangy <hangy@hangy.de>
Date:   Wed Nov 30 23:50:51 2016 +0100

    Workaround for unneeded suffix: Just use the same value.

    The reason for this workaround is that at the method that decides if the suffix should be applied, the information about the time unit is not available. So it would either need to do some string magic and explicitly exclude the "TimeSpanHumanize_MultipleDays" resource key from adding the postfix, or most of the `TimeSpan` formatting code would need to be rewriten for Croatian or modified for all languages to support Croatian. Neither seems reasonable for a quick change.

commit 002cc7b
Author: hangy <hangy@hangy.de>
Date:   Wed Nov 30 23:12:57 2016 +0100

    Added some more tests. From what I've read, all days should resolve to "dana" for the Croatian language. (No "_DualTrialQuadral" postfix.")

commit c190e9d
Author: hangy <hangy@hangy.de>
Date:   Tue Nov 22 00:29:51 2016 +0100

    Add initial test for issue 597.
@clairernovotny
Copy link
Member

clairernovotny commented Dec 2, 2016

Is this ready? btw, the latest dev branch should have build.cmd fixed.

@clairernovotny clairernovotny added this to the vNext milestone Dec 2, 2016
@hangy
Copy link
Contributor Author

hangy commented Dec 2, 2016

Yup, this should be good to go. I'll try the fixed build.cmd during the weekend, thanks! 👍

@clairernovotny clairernovotny merged commit 3399341 into Humanizr:dev Dec 2, 2016
@hangy hangy deleted the issue597 branch December 3, 2016 16:31
@clairernovotny clairernovotny modified the milestones: vNext, v2.2 Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants